Skip to content

Preserve JSX #7387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 31 commits into from
May 5, 2025
Merged

Preserve JSX #7387

merged 31 commits into from
May 5, 2025

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented Apr 10, 2025

@cristianoc, this doesn't work yet, but it highlights the idea. Could you point out where the information is not passed? It's a bit overwhelming for me to grasp.

@nojaf nojaf changed the title Extra app data WIP - Extra app data Apr 10, 2025
@nojaf
Copy link
Collaborator Author

nojaf commented Apr 23, 2025

Got an initial result here, was able to transform:

module ReactDOM = {
    @module("react/jsx-runtime")
    external jsx: (string, JsxDOM.domProps) => Jsx.element = "jsx"
}

let _ = <div className="meh"></div>

into

// Generated by ReScript, PLEASE EDIT WITH CARE
'use strict';

let JsxRuntime = require("react/jsx-runtime");

let ReactDOM = {};

<div className={"meh"}></div>;

exports.ReactDOM = ReactDOM;

There are still a lot of cases to cover, but it is a start.

@nojaf
Copy link
Collaborator Author

nojaf commented Apr 25, 2025

Made some more progress, was able to transform my entire project.
This is an example of how the playground example looks like:

// Generated by ReScript, PLEASE EDIT WITH CARE

import * as React from "react";
import * as JsxRuntime from "react/jsx-runtime";

function Playground$CounterMessage(props) {
  let username = props.username;
  let count = props.count;
  let times = count !== 1 ? (
      count !== 2 ? String(count) + " times" : "twice"
    ) : "once";
  let name = username !== undefined && username !== "" ? username : "Anonymous";
  return <div>{"Hello " + name + ", you clicked me " + times}</div>;
}

let CounterMessage = {
  make: Playground$CounterMessage
};

function Playground$App(props) {
  let match = React.useState(() => 0);
  let setCount = match[1];
  let match$1 = React.useState(() => "Anonymous");
  let setUsername = match$1[1];
  let username = match$1[0];
  return <div>{[
    "Username: ",
    <input type={"text"} value={username} onChange={event => {
      event.preventDefault();
      let eventTarget = event.target;
      let username = eventTarget.value;
      setUsername(_prev => username);
    }}/>,
    <button onClick={_evt => setCount(prev => prev + 1 | 0)}>{"Click me"}</button>,
    <button onClick={_evt => setCount(param => 0)}>{"Reset"}</button>,
    <Playground$CounterMessage count={match[0]} username={username}/>
  ]}</div>;
}

let App = {
  make: Playground$App
};

export {
  CounterMessage,
  App,
}
/* react Not a pure module */

@nojaf nojaf changed the title WIP - Extra app data WIP - Preserve JSX by adding extra app flag Apr 28, 2025
@nojaf nojaf mentioned this pull request Apr 28, 2025
@nojaf nojaf changed the title WIP - Preserve JSX by adding extra app flag [WIP] - Preserve JSX by adding extra app flag Apr 30, 2025
@nojaf nojaf force-pushed the extra-app-data branch from a58f8c9 to d0bfedc Compare May 3, 2025 10:38
Copy link

pkg-pr-new bot commented May 3, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7387

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7387

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7387

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7387

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7387

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7387

commit: aceb0e2

@nojaf
Copy link
Collaborator Author

nojaf commented May 3, 2025

@cknitt you can test this by:

npm i "https://pkg.pr.new/rescript-lang/rescript@f10452935ae8bc0325116323ee9b55f61c1827be"

rescript.json:

  "suffix": ".res.jsx",
  "jsx": {
    "version": 4
  },
 "bsc-flags": [
    "-bs-jsx-preserve"
  ]

(suffix not strictly required)

@nojaf
Copy link
Collaborator Author

nojaf commented May 3, 2025

@cristianoc, this is now functioning effectively. Could you please review this approach? If it seems reasonable, we can refine it.

@nojaf nojaf requested a review from cristianoc May 3, 2025 14:05
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good.
Main question is if there's a way to avoid extending every Lprim with a boolean, and the scope of changes can be restricted somehow.

let fields = ("key", key) :: fields in
print_jsx cxt ~level f fnName tag fields
| _ ->
expression_desc cxt ~level f
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code branch ever executed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is, it means we missed something. Could we add some sort of assert in this case?

@nojaf
Copy link
Collaborator Author

nojaf commented May 4, 2025

@cristianoc could you check e30cab3 and 773124f as well. Had to write some new to deal with prop spreading.

@nojaf nojaf changed the title [WIP] - Preserve JSX by adding extra app flag Preserve JSX May 5, 2025
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor style comments, plus the test does not seem to use preserve mode at the moment?

{
arity = Full;
call_info = Call_ml;
(* no clue if this is correct *) call_transformed_jsx = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use one of the existing default constants for this -- seems likely to be correct as these are generated applications, not coming from jsx ppx

{
arity = Full;
call_info = Call_ml;
(* no clue if this is correct *) call_transformed_jsx =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

(E.call
~info:
{
arity = Full;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about {...one_of_those_constants, transformed_jsx}

{
arity = Full;
call_info = Call_na;
call_transformed_jsx = transformed_jsx;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

~info:
{
arity = Full;
call_info = Call_na;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

~info:
{
arity = Full;
call_info = Call_na;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

arity = Full;
call_info = Call_na;
call_transformed_jsx = transformed_jsx;
}
(E.dot self name) args)
else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

~info:
{
arity = Full;
call_info = Call_na;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

~info:
{
arity = Full;
call_info = Call_na;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

@cristianoc cristianoc marked this pull request as ready for review May 5, 2025 09:05
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go!
Merge away after any final small cleanup.

@nojaf nojaf merged commit 64a29eb into rescript-lang:master May 5, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants